test(source/cloud-sql-mysql): create MCP integration tests#2922
test(source/cloud-sql-mysql): create MCP integration tests#2922dishaprakash wants to merge 31 commits intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive integration tests for the Model Context Protocol (MCP) across various data sources, including AlloyDB, Cloud SQL MySQL, and HTTP. It also adds modular helper functions in tests/mcp_tool.go to facilitate standardized MCP tool testing. Several issues were identified in the review: a critical flaw in the Cloud SQL MySQL instance creation test where mocking http.DefaultClient fails to affect the separate toolbox process, and multiple violations of the repository style guide regarding tool naming conventions (snake_case and product name exclusion). Additionally, the use of time.Sleep in MySQL tests was flagged as a potential source of flakiness, and some error assertions were noted as being too weak.
tests/cloudsqlmysql/cloud_sql_mysql_create_instance_mcp_integration_test.go
Show resolved
Hide resolved
db1827c to
7c651a9
Compare
69bb59b to
d81edf8
Compare
94d7fce to
e3fa5a6
Compare
d81edf8 to
4c21e3f
Compare
0019b41 to
0a48e1c
Compare
16be479 to
f643196
Compare
082b05d to
225d74f
Compare
f643196 to
2e6e044
Compare
2e6e044 to
5a9f6bd
Compare
e698ea1 to
072f36b
Compare
5a9f6bd to
d2bdddd
Compare
1aa40c7 to
0910acf
Compare
| errText = mcpResp.Error.Message | ||
| } else if mcpResp.Result.IsError { | ||
| for _, content := range mcpResp.Result.Content { | ||
| if content.Type == "text" { |
There was a problem hiding this comment.
probably don't need to check this since we only support text.
| if tc.wantBody == "" { | ||
| return | ||
| } |
There was a problem hiding this comment.
for mcp's cases, we might not want to skip these? seems like these are the ones that have content.IsError = true and might have a content.Text value? if needed we can add more field to the test cases struct such as wantContentErr string and set those as the string value.
| supportArrayParam bool | ||
| supportClientAuth bool | ||
| supportSelect1Auth bool | ||
| IsMCP bool |
There was a problem hiding this comment.
| IsMCP bool | |
| isMCP bool |
shouldn't need to be exported. User should always set it with the function.
| wantStatus := tc.wantStatusCode | ||
| // MCP might return 200 OK for some error cases that REST returns 401 | ||
| if wantStatus == http.StatusUnauthorized && mcpStatusCode == http.StatusOK { | ||
| wantStatus = http.StatusOK | ||
| } | ||
| if mcpStatusCode != wantStatus { | ||
| t.Errorf("StatusCode mismatch: got %d, want %d", mcpStatusCode, wantStatus) | ||
| } |
There was a problem hiding this comment.
Seems like the mcp status is always StatusOK. Lets just check if its statusOK. If there are test cases thats not statusOK, we can add a tc.wantStatusCodeMCP.
| wantStatus := tc.wantStatusCode | |
| // MCP might return 200 OK for some error cases that REST returns 401 | |
| if wantStatus == http.StatusUnauthorized && mcpStatusCode == http.StatusOK { | |
| wantStatus = http.StatusOK | |
| } | |
| if mcpStatusCode != wantStatus { | |
| t.Errorf("StatusCode mismatch: got %d, want %d", mcpStatusCode, wantStatus) | |
| } | |
| if mcpStatusCode != http.StatusOK { | |
| t.Error("expected status ok") | |
| } |
| gotObj := getMCPResultText(t, mcpResp) | ||
| gotBytes, _ := json.Marshal(gotObj) | ||
| gotStr := string(gotBytes) | ||
|
|
||
| if strings.HasPrefix(strings.TrimSpace(tc.wantBody), "[") || strings.HasPrefix(strings.TrimSpace(tc.wantBody), "{") { | ||
| // It looks like JSON, let's do JSON comparison | ||
| var gotJSON, wantJSON any | ||
| _ = json.Unmarshal([]byte(gotStr), &gotJSON) | ||
| _ = json.Unmarshal([]byte(tc.wantBody), &wantJSON) | ||
|
|
||
| if got != tc.wantBody { | ||
| t.Fatalf("unexpected value: got %q, want %q", got, tc.wantBody) | ||
| if diff := cmp.Diff(wantJSON, gotJSON); diff != "" { | ||
| t.Fatalf("unexpected JSON value mismatch (-want +got):\n%s\nRaw got: %s\nRaw want: %s", diff, gotStr, tc.wantBody) | ||
| } | ||
| } else { | ||
| // Plain string, use strings.Contains as suggested by user | ||
| if !strings.Contains(gotStr, tc.wantBody) { | ||
| t.Fatalf(`expected %q to contain %q`, gotStr, tc.wantBody) | ||
| } | ||
| } |
There was a problem hiding this comment.
This comparison should be the same as the api endpoint? since they compare direct got != tc.wantBody, we should be able to do the same here too? rather than converting it to type any.
| if !ok { | ||
| t.Fatalf("unable to find result in response body") | ||
| } | ||
| // Extract error text if any |
There was a problem hiding this comment.
For the checks under here, we can check the Content.Text together regardless if it's "IsError".
E.g.
// first parse the content.Text to string
// next check if isError is correct
isError := tc.wantContentErr != ""
if isError != mcpResp.Result.IsError {
// throw t.Fatalf here
}
// check the content.Text string.
if isError {
// check content.Text with tc.wantContentErr
} else {
// check content.Text with tc.wantBody
}
| if resp.StatusCode != tc.wantStatusCode { | ||
| t.Errorf("StatusCode mismatch: got %d, want %d. Response body: %s", resp.StatusCode, tc.wantStatusCode, string(respBody)) | ||
| } | ||
| if configs.IsMCP { |
There was a problem hiding this comment.
did a quick draft --
we'll need to add wantContentErr string to the test case struct.
if configs.IsMCP {
// Invoke the tool via MCP protocol
mcpStatusCode, mcpResp, err := InvokeMCPTool(t, tc.toolName, tc.args, tc.requestHeader)
if err != nil {
t.Fatalf("native error executing %s: %s", tc.toolName, err)
}
if mcpStatusCode != http.StatusOK {
t.Fatalf("expected status ok")
}
if tc.wantContentErr != "" {
AssertMCPError(t, mcpResp, tc.wantContentErr)
return
}
if mcpResp.Result.IsError {
t.Fatalf("%s returned error result: %v", tc.toolName, mcpResp.Result)
}
gotObj := getMCPResultText(t, mcpResp)
gotBytes, err := json.Marshal(gotObj)
if err != nil {
t.Fatalf("error marshaling result object")
}
if string(gotBytes) != tc.wantBody {
var gotJSON, wantJSON any
errGot := json.Unmarshal(gotBytes, &gotJSON)
errWant := json.Unmarshal([]byte(tc.wantBody), &wantJSON)
if errGot == nil && errWant == nil {
if diff := cmp.Diff(wantJSON, gotJSON); diff != "" {
t.Fatalf("unexpected JSON value mismatch (-want +got):\n%s\nRaw got: %s\nRaw want: %s", diff, got, tc.wantBody)
}
} else {
t.Fatalf("unexpected value: got %q, want %q", got, tc.wantBody)
}
}
} else {
Description
This PR migrates the Cloud SQL MySQL integration tests to use the /mcp endpoint on the MCP Toolbox Server.
Key Changes
tests/mcp_tool.go:cloud_sql_mysql_mcp_test.go: Migrated legacy/apitests to/mcpcloud_sql_mysql_create_instance_mcp_test.go: Migrated create-instance tests to validate MCP JSON-RPC handling.tests/options.go: Added functional options for the various MySQL test fixtures to provide an MCP tests specific pathtests/tool.go: Used the above mentioned options to allow calling MCP Endpoints in the test fixtures of MySQL